Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upload Pipeline v2 #5905

Merged
merged 97 commits into from
Jun 8, 2024
Merged

Upload Pipeline v2 #5905

merged 97 commits into from
Jun 8, 2024

Conversation

dtdesign
Copy link
Member

@dtdesign dtdesign commented May 5, 2024

Missing parts:

  • Document the interface for file processors.
  • Proper clean-up of files and thumbnails.
  • Handling of orphaned file uploads that are no longer associated to an objectTypeID.
  • Resizing of GIFs in the browser (see Allow in-browser scaling for non-animated GIFs only / preserve the animation when scaling #4031).
  • Enable downloads again for non-image attachments.
  • Add the missing phrases for the attachment action buttons.
  • Implement a proper error handling for server-side errors, we’re currently just dumping the message property. This might also require some updated CSS to work nicely with the grid layout.
  • Localized error messages for the pre-upload check.
  • Abstract implementation for IFileProcessor that allows us to easily expand the interface later.
  • It is currently only possible to adopt a file but not specific thumbnails. Do we need some kind of callback to register thumbnails in case they’re being referenced directly?
  • Attachment/List.ts handles too much logic and should offload some into Attachment/Entry.ts which is unused right now.
  • There is currently no upload progress because both Firefox and Safari do not support ReadableStream in the request body of fetch(). This means that we cannot track how much data has been sent to the server already. We can still track the chunks but that might be a bit light and potentially misleading due to how different the size of chunks can be for smaller uploads. (UPDATE: The decision was made to ignore this for now. Due to the extreme differences in the values for post_max_size this can differ widely.)
  • <woltlab-core-file-upload> does not support limiting the number of concurrently uploaded files yet.
  • HTTPS is now a hard requirement and must be added to the system requirements. (see Make HTTPS a hard requirement #5919)
  • Add support for a general upper limit for uploaded files that can be configured on the server side / environment.
  • Thumbnails are currently generated as WebP only, do we want to support garbage browsers without WebP support? (UPDATE: The decision was made to not make an exception for ancient browsers.)
  • Write the API documentation for the dev docs. (see 6.1: Document the new upload pipeline docs.woltlab.com#438)
  • Block uploads that exceed the file size limit before posting data to the server.
  • Add a worker to rebuild files / thumbnails.
  • Store the configuration used to generate a thumbnail to skip it if its unchanged and the file hash matches.
  • Migrate the existing attachments to the new system.
  • Remove the rebuild worker for attachments.
  • Add a static WebP variant of the original images for both PNG and JPEG images.
  • Remove the files (PHP+JS), methods and templates for the old attachment system.

Closes #5668

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some remarks from looking at the diff. Didn't attempt to understand the relation between the different parts. Feel free to disregard anything that doesn't make sense or anything you disagree with.

@dtdesign dtdesign mentioned this pull request May 19, 2024
10 tasks
@dtdesign dtdesign marked this pull request as ready for review June 7, 2024 14:53
There is no need to track each chunk because we can simply use the file system as the single source of truth.
Allow for up to 255 chunks and track the state of each uploaded chunks. The `chunks` property is effectively a bitmap whose length represents the number of chunks
This avoids having to buffer the data into separate files which causes a lot of I/O when stitching the file together.
This partially reverts the changes made in bde83e6
The checksum is computed based on the configuration of the thumbnail, allowing to detect when a thumbnail needs to be regenerated. If the checksum matches, the file hash is checked instead to detect damaged files.
@dtdesign dtdesign merged commit e7c871d into master Jun 8, 2024
10 checks passed
@dtdesign dtdesign deleted the upload-pipeline-v2 branch June 8, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Unified upload pipeline
2 participants